I suspect the checks failed due to issues outside of this PR. The initial commit passed all checks. This one only has a README.md update, and spotless:apply locally does not change the file that was committed. How can I retry the checks for the PR, to get the necessary ticks?

Converted back to draft, as no ones assigned yet, to save adding another PR later on, for 2 more set's of devices. Please advise on the first question however - for future reference - looks like it fixed between checkin's.

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/vesync-levoit-binding-help-testing-requested/144175/18

@dag81 i might review this, but before that review, this PR has to build succesfull wihtout new SAT errors or warnigs. Can you sync your branches with openhab main, fix the conflicts and try to build it? Also the license headers should change to 2024.
If you need any help with those steps, please let me know.
Edit: Thing-upgrade instructison have to be added as some existings Things have channels added. See here: https://www.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types

5. Thing upgrade instructions for the added channels are mandatory for this PR.

Hi @lsiepel, Hopefully this finds you well. I was just looking through this and testing on a M4 build against the device I have, I think this likely need's to be merged asap. So will limit the clean-up's to make sure we can hopefully get this in before the cut-off - as otherwise the units switch to % with the new units model for a few bits, as this binding is a old one and my first one. Looking at it I will likely do a deeper pass on it to fully clean it all up, but figured first and most importantly the key is to have it operating like the original binding did, several releases back.
Regarding the point I've quoted - can you give an example of a binding that has these, that I can use as a reference please?

Reference to the documentation: https://www.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types
homewizard is a binding that has these instructions.
The refactoring of this binding should be a seperate PR, als try to prevent breaking changes.

I've opened the PR as Ready, I have just got to look through this and add these bits. I did this in case you wanted to check anything else, while I add this final bit, then its open and ready, to save to time going to and fro. @lsiepel

@lsiepel

I've had a first pass at the upgrade instructions file's. I will have to test them out over the next couple of days. (If its doing what I think that's really nice - I always hated having to say sorry just remote it and re-add then it should appear). In regards to these update scripts my "interpretation" which please correct if I'm wrong, is that the systems caching at a thing id level. So its applying these to its cache used for all defined devices, on boot up, to apply the updates so the channels / updates appear.

What I'm not sure about was the options bit. I've redefined it as I suspect it overwrites whatever it has based on what's defined, so the update to add the pet mode will add it once it runs?

Finally, a few channels types are re-used between two things. As my guess / interpretation is that the system duplicates the data for each thing the instructions are for, so if a typeId is re-used, this should be updated in every thing update definition, as its keyed to those thing's in the cache. Is this correct do you know?

I'll test anyway - but if you know the answers to above that would be great just to find out if its working how I suspect, in order to ensure those instructions have precisely what is required.

I think its ready for another pass now 🤞, so will change the PR status.

Hopefully you had a good weekend, and thanks for all the help in getting these PR's through, hopefully before the next release :)

@lsiepel this one is ready again when you are ready.

@lsiepel - Regarding the bits about the units. That a interesting one. Everyone I've added the unitHint of "one" to, is to ensure it doesn't present the value as a % which the system now appears to do since the UoM model came into play.

Really everything that has had that unitHint added as "one" should generally be a DecimalType rather than a Dimensionless number, to sort this out. However, rather than be inconsistent in the binding I kept it as it was, as this would def. be a breaking change.

Correcting the new one I suspect would actually confuse people more? I could do the whole lot but that would break most peoples configurations at that point, and that's the primarily reason I held off updating them all adding the missing area unit. I'd rather go through them all and update if this is the correct approach and acceptable given the amount it would break config's. In this case I kept the incorrect unit for consistency with rather than implementing a sweeping breaking change.

What do you reckon - align it all with several breaking changes, or keep it consistently wrong?

While reading and thinking about your post, I came to this:
We prefer to have scoped and specific PR’s as that makes it easier to review, reduces riscs and improves the visibility in the release notes.

This PR is not only adding support for some devices but also corrects other things. While greatly appreciated, users won’t expect breaking changes to their 600 device based on the release notes title that states that it add support for 100 device.

That being said I think the breaking changes need to happen and they are relative simple to correct. If possible it is best to extract those changes to a separate PR, if that is ok with you.

@lsiepel I couldn't see how to change the state to request another pass - I think it's good for another pass to see if the changes done and not done seem sensible, whenever you are ready please have a quick look. Thank you again!!! ;)

While reading and thinking about your post, I came to this: We prefer to have scoped and specific PR’s as that makes it easier to review, reduces riscs and improves the visibility in the release notes.

This PR is not only adding support for some devices but also corrects other things. While greatly appreciated, users won’t expect breaking changes to their 600 device based on the release notes title that states that it add support for 100 device.

That being said I think the breaking changes need to happen and they are relative simple to correct. If possible it is best to extract those changes to a separate PR, if that is ok with you.

Yeah I was thinking before of doing a second pass on this one, as I can see further room to clean it up including those channel definitions. I did correct the Numer:Area one to the correct units, so I believe that's the only breaking change. All of the other's, I'll modify to basic Numbers and re-test, for a new PR. I presume this is what you're thinking as well from the comments?

P.S For some reason I only just saw this an hour after you posted it so apologies on the delay replying.

Just adding a comment since the last update. Apologies I had to force-push to over-write a mistaken push from a different local copy, used during testing. On comparing the protocol implementation ported from well over a year ago, it looks like they found some variances for the EU regional model. So this last change is to, add the bespoke behaviour for that variant, and ensure the units are identified correctly. (Another one to look at on my next refactor - cleaning up this identification behaviour further to a prioritised 2-pass approach). Behaviours verified against the spec using the simulator - so this should be fine for the Mist Humidifiers, based on the latest data available.

@lsiepel okay I think I found it - it was the local git config setup that didn't have David Goodyear but dag81, so the DCO check failed. I've updated the latest commit. If its all squashed when merged, I presume it will take the sign-off from that one?

@lsiepel okay I think I found it - it was the local git config setup that didn't have David Goodyear but dag81, so the DCO check failed. I've updated the latest commit. If its all squashed when merged, I presume it will take the sign-off from that one?

Yes, me or another maintainer will pick that commit.